feat(chat): add --timeout option to ask command#260
feat(chat): add --timeout option to ask command#260Flosters wants to merge 2 commits intoteng-lin:mainfrom
Conversation
Long or complex prompts can exceed the default HTTP timeout, causing the request to fail silently. Adding --timeout (default: 120s) lets users increase this for slower responses without changing client-wide settings. Usage: notebooklm ask "summarize everything" --timeout 300
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a configurable --timeout option for the ask_cmd command in the NotebookLM CLI, allowing users to adjust the request timeout for long prompts. The feedback suggests refining the CLI option by using click.IntRange to ensure positive values and removing redundant default text from the help string. Additionally, it is recommended to implement granular timeouts (separating connection and read timeouts) when initializing the NotebookLMClient to improve network resilience.
| @click.option( | ||
| "--timeout", | ||
| default=120, | ||
| type=int, | ||
| show_default=True, | ||
| help="Request timeout in seconds. Increase for long prompts (default: 120).", | ||
| ) |
There was a problem hiding this comment.
The help text contains a redundant default value since show_default=True is enabled, which will cause Click to append [default: 120] automatically. Additionally, the timeout should be validated to ensure it is a positive value to avoid potential issues with the underlying network client.
| @click.option( | |
| "--timeout", | |
| default=120, | |
| type=int, | |
| show_default=True, | |
| help="Request timeout in seconds. Increase for long prompts (default: 120).", | |
| ) | |
| @click.option( | |
| "--timeout", | |
| default=120, | |
| type=click.IntRange(min=1), | |
| show_default=True, | |
| help="Request timeout in seconds. Increase for long prompts.", | |
| ) |
|
|
||
| async def _run(): | ||
| async with NotebookLMClient(client_auth) as client: | ||
| async with NotebookLMClient(client_auth, timeout=float(timeout)) as client: |
There was a problem hiding this comment.
Following the general rules for network clients, it is recommended to use granular timeouts. A single float value sets the same timeout for connection and reading. Using a shorter connect timeout (e.g., 10s) while allowing a longer read timeout improves resilience by failing fast on connection issues while still accommodating slow responses. Note that the NotebookLMClient type hints should also be updated to support this.
| async with NotebookLMClient(client_auth, timeout=float(timeout)) as client: | |
| async with NotebookLMClient(client_auth, timeout=(10.0, float(timeout))) as client: |
References
- For network clients, use granular timeouts with a shorter connect timeout and longer read/write timeouts to improve resilience by detecting connection issues quickly while accommodating slow responses.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/notebooklm/cli/chat.py`:
- Around line 105-111: The --timeout click option currently uses type=int which
permits 0 and negative values; change its validation to require strictly
positive integers by replacing type=int with click.IntRange(min=1) (or add a
callback that raises click.BadParameter if timeout < 1) in the `@click.option`
declaration for "--timeout" in chat.py so the CLI rejects non-positive values at
parse time and returns a clear validation error.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 42881f76-bc63-4350-9e68-f8b5eac381d9
📒 Files selected for processing (1)
src/notebooklm/cli/chat.py
- Use click.IntRange(min=1) instead of type=int to validate positive values at parse time rather than failing at runtime - Remove redundant "(default: 120)" from help text since show_default=True already appends it automatically - Use tuple timeout=(10.0, float(timeout)) for granular connect/read timeouts, providing faster failure detection on connection errors Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
1 participant
Summary
Long or complex prompts can exceed the default HTTP timeout, causing
notebooklm askto fail silently with a connection error. There is currently no way to increase this without changing client-wide settings in code.This PR adds a
--timeoutoption (default: 120s) to theaskcommand, letting users increase it for slower responses.Usage:
Changes
cli/chat.py: add--timeout(int, default 120,show_default=True) toasktimeout=float(timeout)toNotebookLMClient()askinvocation only — no effect on other commandsTest plan
notebooklm ask --helpshows--timeout INTEGERwith default[default: 120]notebooklm ask "question" --timeout 300uses the extended timeoutSummary by CodeRabbit